Add onchain gas cost to trades and orders APIs by jmg-duarte · Pull Request #4540 · cowprotocol/services

jmg-duarte · 2026-06-19T14:11:26Z

Description

The removal of UCP means that we no longer have fees and gas costs info in a single place, meaning the FE must calculate them; the issue comes when one notices that we don't keep track of gas costs for settlements (meaning trades and orders don't either).

This PR adds gas cost tracking, when settlements get indexed, they will now be indexed with the gas cost.
The PR does not do backfills on purpose, that must be done as a separate process.

Changes

DB migrations
OpenAPI updates
DB queries for trades and orders to include gas costs
Update indexing to include gas costs

How to test

Sepolia staging is currently suspended with this code there, please do a trade and review the UI using this link
https://jmgd-ucp.explorer-dev-dxz.pages.dev/

Under costs and fees you should see this:

github-actions · 2026-06-19T14:11:42Z

Reminder: Please update the DB Readme and comment whether migrations are reversible (include rollback scripts if applicable).

If creating new tables, update the tables list.
When adding a new index, consider using CREATE INDEX CONCURRENTLY for tables involved in the critical execution path.
For breaking changes, remember that during rollout k8s starts the new autopilot, runs the Flyway migration, and only then shuts down the old pod. That overlap means the previous version can still be processing requests on the migrated schema, so make it compatible first and ship the breaking DB change in the following release.

Resolved

github-actions · 2026-06-19T14:11:43Z

Reminder: Please consider backward compatibility when modifying the API specification.
If breaking changes are unavoidable, ensure:

You explicitly pointed out breaking changes.
You communicate the changes to affected teams (at least Frontend team and SAFE team).
You provide proper versioning and migration mechanisms.

Caused by:

crates/orderbook/openapi.yml

claude · 2026-06-19T14:11:52Z

Claude finished @jmg-duarte's task in 4m 13s —— View job

PR Review: Add on-chain gas cost to trades and orders APIs

I reviewed the diff against origin/main. The overall approach is solid — sharing the GAS_COST_EXPR between trades and order_gas_costs so they can't drift, and moving the settlement/gas join onto the paginated page CTE (instead of every UNION branch) is a nice performance win. I verified the autopilot stores genuine receipt values (receipt.gas_used, receipt.effective_gas_price), so the "actual on-chain" framing in the OpenAPI is accurate.

I left 3 inline comments:

crates/model/src/order.rs:715 — incorrect doc comment. Says the value is "derived from its quote (gas_amount * gas_price)", but it's actually the on-chain settlement gas cost. The None condition is wrong too.
crates/model/src/trade.rs:34 — same incorrect doc comment, plus the "(e.g. JIT orders)" note is misleading (JIT orders do get a gas cost).
database/sql/V111 — deploy-ordering caveat. The orderbook queries reference the new columns unconditionally; confirm V111 is applied before the new orderbook serves traffic.

The doc-comment mismatches (1 & 2) read like leftovers from an earlier quote-based design — worth fixing since they directly contradict the OpenAPI and the implementation.

Minor (non-blocking)

gas_cost uses skip_serializing_if = "Option::is_none", so the field is omitted when absent, while the OpenAPI marks it nullable: true (implying an explicit null). Consistent with other optional fields here, so likely fine — just flagging the spec/impl nuance for the FE.
DB tests for the rewritten trades query (cargo nextest run postgres ...) aren't runnable in this environment — worth confirming they pass, since the query restructure is substantial.

gemini-code-assist

Code Review

This pull request implements tracking and persistence of actual on-chain gas costs for settlements, trades, and orders, including database migrations, query updates, and OpenAPI documentation. The reviewer feedback highlights that the docstrings for the gas_cost fields in both the OrderMetadata and Trade models are incorrect and misleading, as they describe the value as an estimated cost derived from quotes rather than the actual on-chain gas cost.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

MartinquaXD

Main concern is the error prone settlement <> trade association via log indices and the fact that this estimate can be very much off. Was that discussed with the solver / frontend team?

MartinquaXD · 2026-06-22T08:20:06Z

+    WHERE s.block_number = page.block_number
+    AND   s.log_index > page.log_index


I think deciding to not store the tx_hash in the trades table was a blunder back in the day which leads to this ugly code again and again. Also unless I'm missing something this code does not handle trades from multiple settlements in the same block correctly.
WDYT about introducing the tx_hash column on the trades table to simply join on that? Since we can already get the association using the log index this data can even be fully backfilled using a DB migration alone.

For what it's worth. Same-block different-tx case actually works fine imo. the LATERAL JOIN picks the right settlement per trade and the inner subquery counts the right divisor, I did a dry run for multiple settles within a block case.

The case that does mis-attribute is one tx calling settle() multiple times.
Not sure if this is applicable for now. But in theory, each Settlement event row ends up with the full receipt's gas_used (find_settlement_trace_and_callers only returns the first settle frame in the trace), so per-event splitting sums to 2x the real cost across the tx's trades.

No solver does this afaik, but tx_hash on trades sidesteps the question entirely. So +1 on the direction 🚀

github-actions · 2026-07-17T00:24:43Z

This pull request has been marked as stale because it has been inactive a while. Please update this pull request or it will be automatically closed.

linear-code · 2026-07-22T13:40:06Z

BE-33

jmg-duarte

trying out commenting the PR myself so i can have claude address these

claude · 2026-07-23T16:53:35Z

Claude finished @jmg-duarte's task in 6m 2s —— View job

PR Review: Add on-chain gas cost to trades and orders APIs

I re-reviewed the current state of the branch against origin/main. This is in good shape — the doc-comment issues flagged earlier are now correctly fixed, and the SQL attribution logic is sound.

Verified correct:

The gas-split SQL is self-consistent. GAS_COST_EXPR's divisor (trades between the previous settlement and this one, same block) exactly matches the trade→settlement attribution in the LATERAL join, and it correctly handles multiple settlements in the same block / different txs (confirmed against the new postgres_gas_cost_attribution test — 500/500 and 1500/1500 splits, plus the un-recorded-gas → None case). This addresses @squadgazzz's request for a DB test.
gas_cost now propagates consistently across all order endpoints: single_order/many_orders via full_order_with_quote_into_model_order, and user_orders/orders_for_tx via the orderbook-local full_order_into_model_order (which also delegates and sets it). The shared db_order_conversions::full_order_into_model_order correctly leaves it None for the autopilot's solvable-orders path (which doesn't select the column, defaulted via #[sqlx(default)]). So the earlier "null on /account/{owner}/orders" concern is resolved.
Moving the settlement/gas join onto the paginated page CTE (instead of every UNION branch) is a genuine win — gas is computed only for returned rows. FULL_ORDER_WITH_QUOTE stays off the solvable-orders hot path.

Findings (2 inline):

database/sql/V115 — the deploy-ordering risk is still live since this PR ships the migration and the unconditional query changes together. The orderbook reads s.gas_used before the autopilot-run migration is guaranteed applied. Please confirm the rollout ordering (or split into two releases).
jit_orders.rs get_by_tx — the per-tx endpoint reports the order's lifetime gas cost (summed across all fills), which can over-count in a per-transaction view. Consistent with the field definition, flagging as a nuance for the FE.

Known limitations (equal-split can be inaccurate for mixed-complexity batches; a tx_hash column on trades would simplify this) are already acknowledged in-thread, so I won't re-litigate them. The self-notes on doc-comment conciseness/formatting are yours to tidy.

Read pr-review skill & gather context
Review the diff (git diff origin/main...HEAD)
Verify open review comments are addressed
Post inline comments for findings
Post summary

github-actions · 2026-08-01T00:24:52Z

This pull request has been marked as stale because it has been inactive a while. Please update this pull request or it will be automatically closed.

The settlement observer already reads `gas_used` and `effective_gas_price` from the transaction receipt to build its metrics. Store them on the `settlements` row (columns added in V116) so that consumers can attribute a real on-chain gas cost to settlements. Only settlements observed after this is deployed get values, historical rows stay `NULL`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Expose the settlement gas cost attributed to each fill as `gasCost` (native token wei) on the trades and order-detail endpoints: - a trade gets its settlement's `gas_used * effective_gas_price` split equally across the trades settled in the same transaction - an order gets the sum over all of its fills `null` when the underlying settlement has no recorded gas, i.e. settlements observed before V116 was deployed. Both values are computed in SQL so they come back in the same round-trip as the rest of the order/trade data. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

jmg-duarte requested a review from a team as a code owner June 19, 2026 14:11

gemini-code-assist Bot reviewed Jun 19, 2026

View reviewed changes

Comment thread crates/model/src/order.rs Outdated

Comment thread crates/model/src/trade.rs Outdated

claude Bot reviewed Jun 19, 2026

View reviewed changes

Comment thread crates/model/src/order.rs Outdated

claude Bot reviewed Jun 19, 2026

View reviewed changes

Comment thread crates/model/src/trade.rs Outdated

claude Bot reviewed Jun 19, 2026

View reviewed changes

Comment thread database/sql/V115__add_gas_to_settlements.sql

MartinquaXD reviewed Jun 22, 2026

View reviewed changes

squadgazzz reviewed Jun 22, 2026

View reviewed changes

Comment thread crates/orderbook/src/database/orders.rs Outdated

squadgazzz reviewed Jun 22, 2026

View reviewed changes

Comment thread crates/database/src/trades.rs Outdated

This comment was marked as outdated.

Sign in to view

github-actions Bot added the stale label Jun 30, 2026

jmg-duarte removed the stale label Jul 2, 2026

jmg-duarte marked this pull request as draft July 9, 2026 10:32

github-actions Bot added the stale label Jul 17, 2026

jmg-duarte removed the stale label Jul 23, 2026

jmg-duarte commented Jul 23, 2026

View reviewed changes

jmg-duarte marked this pull request as ready for review July 23, 2026 16:53

claude Bot reviewed Jul 23, 2026

View reviewed changes

Comment thread database/sql/V115__add_gas_to_settlements.sql

claude Bot reviewed Jul 23, 2026

View reviewed changes

Comment thread crates/database/src/jit_orders.rs

github-actions Bot added the stale label Aug 1, 2026

jmg-duarte removed the stale label Aug 3, 2026

jmg-duarte mentioned this pull request Aug 3, 2026

Create columns to track settlement gas info #4697

Merged

jmg-duarte and others added 2 commits August 4, 2026 14:52

jmg-duarte force-pushed the jmgd/order-trade-gas-cost branch from 70a833c to 0063759 Compare August 4, 2026 14:00

jmg-duarte marked this pull request as draft August 4, 2026 14:00

jmg-duarte changed the base branch from main to jmgd/settlement-gas-tracking August 4, 2026 14:01

wip

60e95e6

Base automatically changed from jmgd/settlement-gas-tracking to main August 5, 2026 10:41

jmg-duarte added 3 commits August 5, 2026 11:44

Merge branch 'main' into jmgd/order-trade-gas-cost

110a379

cleanup

1fd10ad

cleanup

26f8085

		WHERE s.block_number = page.block_number
		AND s.log_index > page.log_index

Uh oh!

Conversation

jmg-duarte commented Jun 19, 2026

Description

Changes

How to test

Uh oh!

github-actions Bot commented Jun 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jun 19, 2026

Uh oh!

claude Bot commented Jun 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review: Add on-chain gas cost to trades and orders APIs

Minor (non-blocking)

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MartinquaXD left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MartinquaXD Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

AryanGodara Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

github-actions Bot commented Jul 17, 2026

Uh oh!

linear-code Bot commented Jul 22, 2026

Uh oh!

jmg-duarte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

claude Bot commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review: Add on-chain gas cost to trades and orders APIs

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Aug 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions Bot commented Jun 19, 2026 •

edited

Loading

claude Bot commented Jun 19, 2026 •

edited

Loading

claude Bot commented Jul 23, 2026 •

edited

Loading